Fix unit tests race condition by Danil-Grigorev · Pull Request #340 · openshift/cluster-api-provider-aws

Danil-Grigorev · 2020-07-20T15:58:57Z

In places where the goroutine is spawned, the channels passed into
the methods should not be inherited from the previous block, or in
a parallel environment the variables could be overwritten or closed
by another routine. Passing the variables via argument will ensure
such a thing won't appear, and operations will be atomic.

go test -race -cover ./cmd/... ./pkg/... -count=30

WARNING: DATA RACE
Write at 0x00c0001e9dd0 by goroutine 88:
  runtime.mapassign()
      /usr/lib/golang/src/runtime/map.go:574 +0x0
  k8s.io/apimachinery/pkg/runtime.(*Scheme).AddUnversionedTypes()
      /home/dgrigore/go/pkg/mod/k8s.io/apimachinery@v0.18.2/pkg/runtime/scheme.go:160 +0x2be
  k8s.io/apimachinery/pkg/apis/meta/v1.AddToGroupVersion()
      /home/dgrigore/go/pkg/mod/k8s.io/apimachinery@v0.18.2/pkg/apis/meta/v1/register.go:76 +0x6b2
  github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1.addKnownTypes()
      /home/dgrigore/go/pkg/mod/github.com/openshift/machine-api-operator@v0.2.1-0.20200701225707-950912b03628/pkg/apis/machine/v1beta1/register.go:43 +0x401
  k8s.io/apimachinery/pkg/runtime.(*SchemeBuilder).AddToScheme()
      /home/dgrigore/go/pkg/mod/k8s.io/apimachinery@v0.18.2/pkg/runtime/scheme_builder.go:29 +0x9e
  k8s.io/apimachinery/pkg/runtime.(*SchemeBuilder).AddToScheme-fm()
      /home/dgrigore/go/pkg/mod/k8s.io/apimachinery@v0.18.2/pkg/runtime/scheme_builder.go:27 +0x4b
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.glob..func3()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/termination_suite_test.go:59 +0x278
  github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/internal/leafnodes/runner.go:113 +0xdf
  github.com/onsi/ginkgo/internal/leafnodes.(*runner).run()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/internal/leafnodes/runner.go:64 +0x107
  github.com/onsi/ginkgo/internal/leafnodes.(*simpleSuiteNode).Run()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/internal/leafnodes/suite_nodes.go:25 +0xb2
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runBeforeSuite()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/internal/specrunner/spec_runner.go:123 +0x160
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/internal/specrunner/spec_runner.go:63 +0xcf
  github.com/onsi/ginkgo/internal/suite.(*Suite).Run()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/internal/suite/suite.go:62 +0x69f
  github.com/onsi/ginkgo.RunSpecsWithCustomReporters()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/ginkgo_dsl.go:226 +0x338
  github.com/onsi/ginkgo.RunSpecsWithDefaultAndCustomReporters()
      /home/dgrigore/go/pkg/mod/github.com/onsi/ginkgo@v1.12.0/ginkgo_dsl.go:214 +0x11f
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.TestReconciler()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/termination_suite_test.go:50 +0x138
  testing.tRunner()
      /usr/lib/golang/src/testing/testing.go:909 +0x199

Previous read at 0x00c0001e9dd0 by goroutine 198:
  runtime.mapaccess2()
      /usr/lib/golang/src/runtime/map.go:453 +0x0
  k8s.io/apimachinery/pkg/runtime.(*Scheme).ObjectKinds()
      /home/dgrigore/go/pkg/mod/k8s.io/apimachinery@v0.18.2/pkg/runtime/scheme.go:260 +0x38c
  k8s.io/apimachinery/pkg/runtime.(*parameterCodec).EncodeParameters()
      /home/dgrigore/go/pkg/mod/k8s.io/apimachinery@v0.18.2/pkg/runtime/codec.go:191 +0x88
  k8s.io/client-go/rest.(*Request).SpecificallyVersionedParams()
      /home/dgrigore/go/pkg/mod/k8s.io/client-go@v0.18.2/rest/request.go:351 +0xe2
  sigs.k8s.io/controller-runtime/pkg/client.(*typedClient).List()
      /home/dgrigore/go/pkg/mod/k8s.io/client-go@v0.18.2/rest/request.go:344 +0x395
  sigs.k8s.io/controller-runtime/pkg/client.(*client).List()
      /home/dgrigore/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.0/pkg/client/client.go:174 +0x147
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.(*handler).getMachineForNode()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/handler.go:133 +0x1be
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.(*handler).run()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/handler.go:92 +0xcd
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.(*handler).Run.func1()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/handler.go:74 +0x72

Goroutine 88 (running) created at:
  testing.(*T).Run()
      /usr/lib/golang/src/testing/testing.go:960 +0x651
  testing.runTests.func1()
      /usr/lib/golang/src/testing/testing.go:1202 +0xa6
  testing.tRunner()
      /usr/lib/golang/src/testing/testing.go:909 +0x199
  testing.runTests()
      /usr/lib/golang/src/testing/testing.go:1200 +0x521
  testing.(*M).Run()
      /usr/lib/golang/src/testing/testing.go:1117 +0x2ff
  main.main()
      _testmain.go:92 +0x337

Goroutine 198 (finished) created at:
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.(*handler).Run()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/handler.go:73 +0x163
  sigs.k8s.io/cluster-api-provider-aws/pkg/termination.StartTestHandler.func1()
      /home/dgrigore/go/src/github.com/openshift/cluster-api-provider-aws/pkg/termination/termination_suite_test.go:85 +0x48

CI failure example:

 goroutine 823 [running]:
	goroutine running on other thread; stack unavailable
created by sigs.k8s.io/cluster-api-provider-aws/pkg/termination.(*handler).Run
	/go/src/sigs.k8s.io/cluster-api-provider-aws/pkg/termination/handler.go:73 +0x164
goroutine 866 [select]:
sigs.k8s.io/cluster-api-provider-aws/pkg/termination.(*handler).Run(0xc00053ca50, 0xc00022cde0, 0x0, 0x0)
	/go/src/sigs.k8s.io/cluster-api-provider-aws/pkg/termination/handler.go:77 +0x230
sigs.k8s.io/cluster-api-provider-aws/pkg/termination.StartTestHandler.func1(0xc00022ce40, 0x22137a0, 0xc00053ca50, 0xc00022cde0)
	/go/src/sigs.k8s.io/cluster-api-provider-aws/pkg/termination/termination_suite_test.go:85 +0x49
created by sigs.k8s.io/cluster-api-provider-aws/pkg/termination.StartTestHandler
	/go/src/sigs.k8s.io/cluster-api-provider-aws/pkg/termination/termination_suite_test.go:84 +0xab

In places where the goroutine is spawned, the channels passed into the methods should not be inherited from previous block, or in a parallel environment the variables could be overritten or closed by another routine. Passing the variables via argument will ensure such thing won't appear, and operations will be atomic.

openshift-ci-robot · 2020-07-20T15:59:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign danil-grigorev
You can assign the PR to them by writing /assign @danil-grigorev in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-07-20T16:19:45Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	`2857821`	link	`/test unit`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

JoelSpeed · 2020-07-21T09:35:06Z

I think this race may actually be fixed by #337 (I saw the error before and included a "fix"(?) for it in that PR)

Danil-Grigorev · 2020-08-31T11:04:31Z

/close fixed with #337

openshift-ci-robot requested review from alexander-demicev and michaelgugino July 20, 2020 15:59

Danil-Grigorev mentioned this pull request Jul 20, 2020

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle #339

Merged

Danil-Grigorev mentioned this pull request Jul 22, 2020

Ensure we are testing scheme registration for termination handler #337

Merged

Danil-Grigorev closed this Aug 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unit tests race condition#340

Fix unit tests race condition#340
Danil-Grigorev wants to merge 1 commit intoopenshift:masterfrom
Danil-Grigorev:race-termination-handler

Danil-Grigorev commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 20, 2020

Uh oh!

JoelSpeed commented Jul 21, 2020

Uh oh!

Danil-Grigorev commented Aug 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

Danil-Grigorev commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 20, 2020

Uh oh!

JoelSpeed commented Jul 21, 2020

Uh oh!

Danil-Grigorev commented Aug 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments